Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override default Talkback automatic content grouping and generate a custom contentDescription #33690

Closed
wants to merge 61 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Apr 22, 2022

Summary

The Implementation of the functionality consists of:

  1. Checking that an element has no contentDescription and no text and has other content to announce (role, state, etc.) which causes this issue (for ex. the accessibilityRole is announced before the contentDescription for ex. "Button, My text children component")
  2. If Talkback finds no content to announce on the current node, a custom contentDescription is generated from the child elements that respect the following conditions:

If an AccessibilityNodeInfo is considered "actionable" (which Talkback defines as having clickable=true, longClickable=true, or focusable=true, or having AccessibilityActions for any of those), AND it has some content to read like a contentDescription or text, it will be considered focusable.
If an AccessibilityNodeInfo is considered "actionable" AND it does not have content to read like a contentDescription or text Talkback will parse descendant elements looking for non-focusable descendants to use as content.

  1. implementation of a method getTalkbackDescription to generate the above contentDescription from child elements
  2. over-ride parent contentDescription (accessibilityLabel) with the value returned from getTalkbackDescription

Related notes on Android: Role description is announced before the components text, rather than after #31042. This issue fixes #31042.

Changelog

[Android] [Added] - Override default Talkback automatic content grouping and generate a custom contentDescription

Test Plan

PR Branch
1. Screenreader correctly announcing accessible/non-accessible items (link)
2. Screenreader announces Pressability items (link)
3. Button role is announced after child contentDescription with TouchableNativeFeedback (link)
4. Testing for regressions in Accessibility Actions (link)
5. Screenreader focuses on ScrollView Items (link)
6. Recordings of complete test cases in rn-tester app main and pr branch (link)
9. TouchableOpacity with TextInput child announces contentDescription before the Role (link)
10. One of the child has accessibilityState (hasStateDescription triggers the announcement) (link)
11. One of the child has accessibilityHint (hasText triggers the announcement) (link)

Main
15. The View does not announce the child component Text when accessibilityLabel is missing (automatic content grouping) (link)
8. TouchableOpacity with child EditText annouces placeholder text before and after the role (link)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 22, 2022
@react-native-bot react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Apr 22, 2022
@analysis-bot
Copy link

analysis-bot commented Apr 22, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 9e169da
Branch: main

@analysis-bot
Copy link

analysis-bot commented Apr 22, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,620,789 +2,735
android hermes armeabi-v7a 7,035,043 +2,754
android hermes x86 7,920,843 +2,717
android hermes x86_64 7,894,463 +2,708
android jsc arm64-v8a 9,497,831 +2,429
android jsc armeabi-v7a 8,275,452 +2,437
android jsc x86 9,435,625 +2,406
android jsc x86_64 10,028,609 +2,403

Base commit: 9e169da
Branch: main

@blavalla
Copy link
Contributor

I think overall this approach will work, but my concern is that we will be doing this work unnecessarily for 90% of elements that happen to trigger it. As in, Talkback would be doing identical work and an identical outcome, so we might as well delegate to its behavior wherever we can.

It's only in cases where an element has no contentDescription or text and happens to have some other content to announce (role, state, etc.) that this behavior needs to be triggered. I think you can probably add some logic to detect this specific scenario with what you've built already, I just wanted to call out that point #1 isn't as simple as just checking for lack of a contentDescription.

… the announcement)

from facebook#33690 (comment)

I noticed that getStateDescription returns null even when adding accessibilityState. Calling setStateDescription will update the state description, which is not set via the react accessibilityState or other props.

Seems that the method [setViewState](https://github.com/fabriziobertoglio1987/react-native/blob/ad6732aa3576786d37478a729b112031dd94b682/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java#L254) does not call setStateDescription.

I will further investigate tomorrow.

<details><summary>View#setStateDescription</summary>
<p>

```java
    /**
     * Sets the {@link View}'s state description.
     * <p>
     * A state description briefly describes the states of the view and is primarily used
     * for accessibility support to determine how the states of a view should be presented to
     * the user. It is a supplement to the boolean states (for example, checked/unchecked) and
     * it is used for customized state description (for example, "wifi, connected, three bars").
     * State description changes frequently while content description should change less often.
     * State description should be localized. For android widgets which have default state
     * descriptions, app developers can call this method to override the state descriptions.
     * Setting state description to null restores the default behavior.
     *
     * @param stateDescription The state description.
     * @see #getStateDescription()
     */
    @RemotableViewMethod
    public void setStateDescription(@nullable CharSequence stateDescription) {
        if (mStateDescription == null) {
            if (stateDescription == null) {
                return;
            }
        } else if (mStateDescription.equals(stateDescription)) {
            return;
        }
        mStateDescription = stateDescription;
        if (!TextUtils.isEmpty(stateDescription)
                && getImportantForAccessibility() == IMPORTANT_FOR_ACCESSIBILITY_AUTO) {
            setImportantForAccessibility(IMPORTANT_FOR_ACCESSIBILITY_YES);
        }
        if (AccessibilityManager.getInstance(mContext).isEnabled()) {
            AccessibilityEvent event = AccessibilityEvent.obtain();
            event.setEventType(AccessibilityEvent.TYPE_WINDOW_CONTENT_CHANGED);
            event.setContentChangeTypes(AccessibilityEvent.CONTENT_CHANGE_TYPE_STATE_DESCRIPTION);
            sendAccessibilityEventUnchecked(event);
        }
    }
```

</p>
</details>

This is the implementation of TalkBack [hasStateDescription](https://github.com/google/talkback/blob/6c0b475b7f52469e309e51bfcc13de58f18176ff/utils/src/main/java/com/google/android/accessibility/utils/AccessibilityNodeInfoUtils.java#L1672-L1677).

The following fields of the view are set with the accessibilityState:

- [checkable](https://github.com/fabriziobertoglio1987/react-native/blob/ad6732aa3576786d37478a729b112031dd94b682/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactAccessibilityDelegate.java#L411) => BOOLEAN_PROPERTY_CHECKABLE
- [enabled](https://github.com/fabriziobertoglio1987/react-native/blob/ad6732aa3576786d37478a729b112031dd94b682/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactAccessibilityDelegate.java#L408) => BOOLEAN_PROPERTY_ENABLED

<details><summary>setCheckable</summary>
<p>

https://github.com/aosp-mirror/platform_frameworks_base/blob/19e53cfdc8a5c6ef45c0adf2dd239576ddce5822/core/java/android/view/accessibility/AccessibilityNodeInfo.java#L2008

```java
  /**
   * Sets whether this node is checkable.
   * <p>
   *   <strong>Note:</strong> Cannot be called from an
   *   {@link android.accessibilityservice.AccessibilityService}.
   *   This class is made immutable before being delivered to an AccessibilityService.
   * </p>
   *
   * @param checkable True if the node is checkable.
   *
   * @throws IllegalStateException If called from an AccessibilityService.
   */
  public void setCheckable(boolean checkable) {
      setBooleanProperty(BOOLEAN_PROPERTY_CHECKABLE, checkable);
  }
```

</p>
</details>

<details><summary>setEnabled</summary>
<p>

https://github.com/aosp-mirror/platform_frameworks_base/blob/19e53cfdc8a5c6ef45c0adf2dd239576ddce5822/core/java/android/view/accessibility/AccessibilityNodeInfo.java#L2227

```java
    /**
     * Sets whether this node is enabled.
     * <p>
     *   <strong>Note:</strong> Cannot be called from an
     *   {@link android.accessibilityservice.AccessibilityService}.
     *   This class is made immutable before being delivered to an AccessibilityService.
     * </p>
     *
     * @param enabled True if the node is enabled.
     *
     * @throws IllegalStateException If called from an AccessibilityService.
     */
    public void setEnabled(boolean enabled) {
        setBooleanProperty(BOOLEAN_PROPERTY_ENABLED, enabled);
    }
```

</p>
</details>

The implementation of `hasStateDescription` is still valid
@fabOnReact fabOnReact marked this pull request as ready for review June 8, 2022 14:15
facebook-github-bot pushed a commit that referenced this pull request Aug 10, 2022
…34245)

Summary:
Previously published by [grgr-dkrk][2] as [https://github.com/facebook/react-native/issues/31296][3], fixes the following issues:

1) ImportantForAccessibility="no" does not work on Text, Button
2) ImportantForAccessibility="no-hide-descendants" does not work on Text, Button, or ImageBackground.

Note: On ImageBackground, focus is prevented on the imageBackground node itself, but focus is not prevented on its descendants.

Note: [Button component expected behavior for prop importantForAccessibility][4]
>Some components like Button seem like atomic units, but they end up rendering a hierarchy of components for example a wrapper View with a Text inside them. Allowing those descendants to become focusable, breaks the model of these being a single component to consider and forcing no-hide-descendants makes sense here.

>The other option is always to render any descendants of these elements with importantForAccessibility="no", so they can never be focusable on their own. This would have the same result, **BUT may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label** (which is what Talkback does with unfocusable text elements by default).

Note: [importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images][5]

fixes #30850 related #33690

## Changelog

[Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground

Pull Request resolved: #34245

Test Plan:
1) testing ImageBackground importantForAccessiblity ([link to test][1])
2) importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images ([link to test][5])
3) testing ImageBackground importantForAccessiblity ([link to test][6])
4) Button with importantForAccessibility=no ([link to test][7])

[1]: #31296 (comment) ""
[2]: https://github.com/grgr-dkrk "grgr-dkrk"
[3]: #31296 "#31296"
[4]: #31296 (comment) "expected behaviour with prop importantForAccessibility in Button component"
[5]: #30850 (comment) "importantForAccessibility=no does not allow screenreader focus on nested Text Components with accessibilityRole=link or inline Images"
[6]: #34245 (comment) "testing ImageBackground importantForAccessiblity"
[7]: #34245 (comment) "Button with importantForAccessibility=no"

Reviewed By: cipolleschi

Differential Revision: D38121992

Pulled By: dmitryrykun

fbshipit-source-id: 368b4dcb47d7940274820aa2e39ed5e2ca068821
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…acebook#34245)

Summary:
Previously published by [grgr-dkrk][2] as [https://github.com/facebook/react-native/issues/31296][3], fixes the following issues:

1) ImportantForAccessibility="no" does not work on Text, Button
2) ImportantForAccessibility="no-hide-descendants" does not work on Text, Button, or ImageBackground.

Note: On ImageBackground, focus is prevented on the imageBackground node itself, but focus is not prevented on its descendants.

Note: [Button component expected behavior for prop importantForAccessibility][4]
>Some components like Button seem like atomic units, but they end up rendering a hierarchy of components for example a wrapper View with a Text inside them. Allowing those descendants to become focusable, breaks the model of these being a single component to consider and forcing no-hide-descendants makes sense here.

>The other option is always to render any descendants of these elements with importantForAccessibility="no", so they can never be focusable on their own. This would have the same result, **BUT may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label** (which is what Talkback does with unfocusable text elements by default).

Note: [importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images][5]

fixes facebook#30850 related facebook#33690

## Changelog

[Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground

Pull Request resolved: facebook#34245

Test Plan:
1) testing ImageBackground importantForAccessiblity ([link to test][1])
2) importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images ([link to test][5])
3) testing ImageBackground importantForAccessiblity ([link to test][6])
4) Button with importantForAccessibility=no ([link to test][7])

[1]: facebook#31296 (comment) ""
[2]: https://github.com/grgr-dkrk "grgr-dkrk"
[3]: facebook#31296 "facebook#31296"
[4]: facebook#31296 (comment) "expected behaviour with prop importantForAccessibility in Button component"
[5]: facebook#30850 (comment) "importantForAccessibility=no does not allow screenreader focus on nested Text Components with accessibilityRole=link or inline Images"
[6]: facebook#34245 (comment) "testing ImageBackground importantForAccessiblity"
[7]: facebook#34245 (comment) "Button with importantForAccessibility=no"

Reviewed By: cipolleschi

Differential Revision: D38121992

Pulled By: dmitryrykun

fbshipit-source-id: 368b4dcb47d7940274820aa2e39ed5e2ca068821
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…acebook#34245)

Summary:
Previously published by [grgr-dkrk][2] as [https://github.com/facebook/react-native/issues/31296][3], fixes the following issues:

1) ImportantForAccessibility="no" does not work on Text, Button
2) ImportantForAccessibility="no-hide-descendants" does not work on Text, Button, or ImageBackground.

Note: On ImageBackground, focus is prevented on the imageBackground node itself, but focus is not prevented on its descendants.

Note: [Button component expected behavior for prop importantForAccessibility][4]
>Some components like Button seem like atomic units, but they end up rendering a hierarchy of components for example a wrapper View with a Text inside them. Allowing those descendants to become focusable, breaks the model of these being a single component to consider and forcing no-hide-descendants makes sense here.

>The other option is always to render any descendants of these elements with importantForAccessibility="no", so they can never be focusable on their own. This would have the same result, **BUT may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label** (which is what Talkback does with unfocusable text elements by default).

Note: [importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images][5]

fixes facebook#30850 related facebook#33690

## Changelog

[Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground

Pull Request resolved: facebook#34245

Test Plan:
1) testing ImageBackground importantForAccessiblity ([link to test][1])
2) importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images ([link to test][5])
3) testing ImageBackground importantForAccessiblity ([link to test][6])
4) Button with importantForAccessibility=no ([link to test][7])

[1]: facebook#31296 (comment) ""
[2]: https://github.com/grgr-dkrk "grgr-dkrk"
[3]: facebook#31296 "facebook#31296"
[4]: facebook#31296 (comment) "expected behaviour with prop importantForAccessibility in Button component"
[5]: facebook#30850 (comment) "importantForAccessibility=no does not allow screenreader focus on nested Text Components with accessibilityRole=link or inline Images"
[6]: facebook#34245 (comment) "testing ImageBackground importantForAccessiblity"
[7]: facebook#34245 (comment) "Button with importantForAccessibility=no"

Reviewed By: cipolleschi

Differential Revision: D38121992

Pulled By: dmitryrykun

fbshipit-source-id: 368b4dcb47d7940274820aa2e39ed5e2ca068821
</TouchableNativeFeedback>
</RNTesterBlock>

<RNTesterBlock title="The child is not TextInput, the contentDescription is not empty and does not have nodeText">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove duplicate example (this is the same as the previous one)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit ddfb345

</TouchableNativeFeedback>
</RNTesterBlock>

<RNTesterBlock title="One of the child has accessibilityState (hasStateDescription triggers the announcement)">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example feels a bit odd to me. The state itself is not concatenated into the announcement, which makes sense, as it would lead to potentially announcing conflicting state if two child elements had separate states like "checked" and "unchecked", but because of this I'm not sure what value this example is providing. If there was no state here and just the label it would work the exact same way.

I think we can probably just remove this one and maybe replace it with one where the parent element has a state and making sure that state is reflected even when the children are combined into it.

Copy link
Contributor Author

@fabOnReact fabOnReact Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @blavalla. I did the changes with commit ddfb345

I improved and moved the example before One of the children has accessibilityLabel, role, state, and accValue

Talkback only pulls the child's contentDescription or text but does not include the child's accessibilityState or accessibilityRole. TalkBack avoids announcements of conflicting states or roles (for example, 'button' and 'slider').

Screen Shot 2022-08-30 at 19 03 29

<RNTesterBlock title="Parent and children have different role">
<TouchableNativeFeedback
accessible={true}
importantForAccessibility="yes"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think importantForAccessibility="yes" is necessary for any of these examples, so lets remove it on all of them just to make it clear that this isn't a prequalification for this feature working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit ddfb345

@fabOnReact fabOnReact self-assigned this Aug 30, 2022
@fabOnReact fabOnReact marked this pull request as draft August 30, 2022 09:55
This example feels a bit odd to me. The state itself is not concatenated into the announcement, which makes sense, as it would lead to potentially announcing conflicting state if two child elements had separate states like "checked" and "unchecked", but because of this I'm not sure what value this example is providing. If there was no state here and just the label it would work the exact same way.
I think we can probably just remove this one and maybe replace it with one where the parent element has a state and making sure that state is reflected even when the children are combined into it.
facebook#33690 (comment)
facebook#33690 (comment)

I don't think importantForAccessibility="yes" is necessary for any of these examples, so lets remove it on all of them just to make it clear that this isn't a prequalification for this feature working.
facebook#33690 (comment)

Remove duplicate example (this is the same as the previous one)
facebook#33690 (comment)
@fabOnReact fabOnReact marked this pull request as ready for review August 30, 2022 11:41
@facebook-github-bot
Copy link
Contributor

@blavalla has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

This pull request was successfully merged by @fabriziobertoglio1987 in 759056b.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Dec 2, 2022
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…ustom contentDescription (facebook#33690)

Summary:
The Implementation of the functionality consists of:

1) Checking that an element has no contentDescription and no text and has other content to announce (role, state, etc.) which causes this issue (for ex. the accessibilityRole is announced before the contentDescription for ex. "Button, My text children component")
2) If Talkback finds no content to announce on the current node, a custom contentDescription is generated from the child elements that respect the following conditions:

>If an AccessibilityNodeInfo is considered "actionable" (which Talkback defines as having clickable=true, longClickable=true, or focusable=true, or having AccessibilityActions for any of those), AND it has some content to read like a contentDescription or text, it will be considered focusable.
>If an AccessibilityNodeInfo is considered "actionable" AND it does not have content to read like a contentDescription or text Talkback will parse descendant elements looking for non-focusable descendants to use as content.

3) implementation of a method getTalkbackDescription to generate the above contentDescription from child elements
4) over-ride parent contentDescription (accessibilityLabel) with the value returned from getTalkbackDescription

Related [notes on Android: Role description is announced before the components text, rather than after https://github.com/facebook/react-native/issues/31042][51]. This issue fixes [https://github.com/facebook/react-native/issues/31042][50].

## Changelog

[Android] [Added] - Override default Talkback automatic content grouping and generate a custom contentDescription

Pull Request resolved: facebook#33690

Test Plan:
**PR Branch**
[1]. Screenreader correctly announcing accessible/non-accessible items ([link][1])
[2]. Screenreader announces Pressability items ([link][2])
[3]. Button role is announced after child contentDescription with TouchableNativeFeedback ([link][3])
[4]. Testing for regressions in Accessibility Actions ([link][4])
[5]. Screenreader focuses on ScrollView Items ([link][5])
[6]. Recordings of complete test cases in rn-tester app main and pr branch ([link][6])
[9]. TouchableOpacity with TextInput child announces contentDescription before the Role ([link][9])
[10]. One of the child has accessibilityState (hasStateDescription triggers the announcement) ([link][10])
[11]. One of the child has accessibilityHint (hasText triggers the announcement) ([link][11])

**Main**
[15]. The View does not announce the child component Text when accessibilityLabel is missing (automatic content grouping) ([link][15])
[8]. TouchableOpacity with child EditText annouces placeholder text before and after the role ([link][8])

[1]: fabOnReact/react-native-notes#14 (comment) "Screenreader correctly announcing accessible/non-accessible items"
[2]: fabOnReact/react-native-notes#14 (comment) "Screenreader announces Pressability items"
[3]: fabOnReact/react-native-notes#14 (comment) "Button role is announced after child contentDescription"
[4]: fabOnReact/react-native-notes#14 (comment) "Testing for regressions in Accessibility Actions"
[5]: fabOnReact/react-native-notes#14 (comment) "Screenreader focuses on ScrollView Items"
[6]: fabOnReact/react-native-notes#14 (comment) "Recordings of complete test cases in rn-tester app main and pr branch"
[7]: fabOnReact/react-native-notes#14 (comment) "TouchableOpacity with child EditText annouces Button role before the child contentDescription"
[8]: fabOnReact/react-native-notes#14 (comment) "TouchableOpacity with child EditText annouces placholder text before and after the role"
[9]: fabOnReact/react-native-notes#14 (comment) "TouchableOpacity with TextInput child announces contentDescription before the Role"
[10]: fabOnReact/react-native-notes#14 (comment) "One of the child has accessibilityState (hasStateDescription triggers the announcement)"
[11]: fabOnReact/react-native-notes#14 (comment) "One of the child has accessibilityHint (hasText triggers the announcement)"

[15]: fabOnReact/react-native-notes#14 (comment) "The View does not announce the child component Text when accessibilityLabel is missing (automatic content grouping)"

[50]: facebook#31042 "Android: Role description is announced before the components text, rather than after facebook#31042"
[51]: fabOnReact/react-native-notes#14 "notes on Android: Role description is announced before the components text, rather than after facebook#31042"

Reviewed By: cipolleschi

Differential Revision: D39177512

Pulled By: blavalla

fbshipit-source-id: 6bd0fba9c347bc14b3839e903184c86d2bcab3d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: Role description is announced before the components text, rather than after
6 participants